fix: redefine the trait inheritance behavior #532

Fannon · 2021-04-29T14:18:43Z

title: Redefine and explicitly formalize the trait inheritance behavior

Related issue(s): #505

Related PR(s): #517 (will be deprecated by this PR)

Code Example: https://jsfiddle.net/FannonF/yLsonr57/latest/

The basic issue is that the current defined trait merging behavior would overwrite the target object with the traits.
This might not what you would expect from trait inheritance and potentially limits the usefulness of traits, as they can't be used for inheritance use-cases.

There are three options that are proposed:

A) Do not change the current trait behavior and just describe it and it's limitations (fix: clarify that traits overwrite the objects they are applied to #517)
- PRO: No breaking change to the spec. Just clarifications.
- CON: The current trait behavior makes inheritance use-cases impossible. It can be used for composition, but needs to be carefully handled so nothing is accidentally overwritten without the creator realizing it.
B) Change the trait behavior of the spec by changing the order of the merging / patching
- Proposal: Make the target of where the trait is applied to the "most specific" and not the "least specific", so it is never overwritten.
- PRO: Now we support inheritance like use-cases. We still can do composition.
- CON: We break the use-case where we explicitly wan't to overwrite the target object.
- If we implement this, I see two ways how to go about it
  - B1) Based on JSON merge patch (as we do already)
    - Pro: Make use of an existing standard, including existing libraries
    - Con: The goal and semantics of a merge patch are different than what we want to do with trait inheritance. We are re-using a standard for a different purposes than originally intended.
  - B2) Based on a generic object deep merge algorithm that we clearly define.
    - Behaves similar to JSON merge patch, but likely without the null behavior
    - I'd highly recommend to not merge arrays. Just overwriting is is the simplest solution and keeps the spec simple. While for some use-cases this might be a bit inconvenient, it gives the user full control.
    - Pro: No "dependency" to another standards or libraries. We can describe the algorithm rather shortly (like RAML) and provide a reference implementation (see my code examples).
  - Con: This needs to be custom implemented and there might be no existing implementation for your ecosystem.
C) Consider not supporting such traits / inheritance in a future AsyncAPI version (drop feature)
- If the goal is to keep the spec itself simple and also the consumer side, we might event want to drop more complicated features
- As an alternative, we move such functionality to a pre-processor stage, which the AsyncAPI providers are responsible for themselves. There they can use template languages, traits, inheritance as they like if they implement it on their own to their own expectations. In the simplest case, you can write the spec in YAML, use features like anchors and merge keys, parse and serialize it to a "simple", denormalized YAML / JSON file and be done with it.

In case we go for an option in the "B range", we have to decide:

Change the current behavior of the trait inheritance ?
- This would either be a breaking change to the bug or if we stretch it a "bugfix" if we consider the current behavior as unintended (which I think we don't do after discussions with @fmvilas )
Keep the current trait implementation and add a new feature besides of it (as a new property?)
- This would further increase the complexity but also the flexibility of the spec
- This is a breaking change for consumers and tools, because they now need to support the new inheritance feature or else they would interpret the AsyncAPI docs wrong / incompletely. For AsyncAPI providers, it's a compatible change (the documents do not need to be migrated)

For a detailed description and discussion of the problem, see #505 and its comments.

Related Material / Inspiration

RAML Traits:
- Specification: https://github.com/raml-org/raml-spec/blob/master/versions/raml-10/raml-10.md/#algorithm-of-merging-traits-and-methods
- They have defined and described their custom trait (merge) algorithm as part of the spec
- It looks like a RAML trait does not overwrite the target, though

Open Questions / Problems

Do we consider changing the trait merge behavior or do we want to keep it as it is?
Do we consider changing the merge behavior as a breaking change? How do we go about introducing it to the spec?
Should we stick with the JSON Merge Patch merge behavior?
- The semantics and the use-case of JSON Merge Patch is quite different from the trait merge use-case. This may lead to confusions (and it already has)
- JSON Merge Patch has a special behavior on merging null values, which may not be useful / expected for trait inheritance

update to origin master

Now it aims to be identical to JSON Merge patch

Added some further clarifications to avoid target object naming confusion

sonarcloud · 2021-05-01T05:15:41Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

fmvilas · 2021-05-14T12:08:30Z

spec/asyncapi.md

+
+When traits ([Operation Trait Object](#operationTraitObject) and [Message Trait Object](#messageTraitObject)) are defined, they MUST be merged into the target object according to the following rules:
+
+* Traits are merged with a recursive merge algorithm, as defined in the [JSON Merge Patch](https://tools.ietf.org/html/rfc7386) RFC. To summarize the main merge rules as a merge from source object into a target object:


I'm confused now. If this is as defined in JSON Merge Patch, why do we have custom rules below? I'm also having a hard time figuring out how this would work. Maybe we could have some examples along with the explanation?

Yes, I'm also unsure about that.

So the RFC already states the merge rules, but it also covers many other aspects that are not relevant to this trait inheritance at all. If you get the rules right, you could just state a few rules here. I still thinks its valuable to say that this is RFC-7386 conformant, so you can reuse existing libraries.

In the linked issue, I've referenced some examples and code. I wasn't sure if this should be part of the specification itself? But having some examples and code is always helful if you can link to it.

See https://jsfiddle.net/FannonF/yLsonr57/27/

@fmvilas Btw. some of those rules are still necessary because they are not part of the JSON Merge Patch RFC.

The following rules are specific to AsyncAPI itself and have been missing (undefined) previously:

Before applying merges, JSON Schema $ref MUST be dereferenced

The Message Object and the Operation Object that the trait is applied to is the most specific and is therefore never overwritten, only extended.

The following rule is AsyncAPI specific but was already there before I think:

The order of the traits defines their specificity. Subsequent traits in the trait array are more specific.

Maybe we should just explicitly state the extension to the RFC? 🤔 That way we ensure there is less interpretation involved.

Something similar to:

The traits ([Operation Trait Object](#operationTraitObject) and [Message Trait Object](#messageTraitObject)) are defined using [JSON Merge Patch](https://tools.ietf.org/html/rfc7386). In extension to these merge rules, the following applies: - Any references in traits, must be resolved before applying them. - The Message Object and the Operation Object that the trait is applied to is the most specific and is therefore never overwritten, only extended. - The order of the traits defines their specificity. Subsequent traits in the trait array are more specific. ...

For your coding examples, do you mind writing a comment in this issue: asyncapi/parser-api#36

This way we can include your examples as documentation and explanation of how to achieve the correct behavior in tooling 🙂

Hi Jonas,

sure, I can do that! However, we were not completely agreeing on what the future approach should be.
This code example was my take on how I would do it ;)

Best,
Simon

Ahh yea, my bad, I saw this PR as an attempt to clarify the current behavior. Gonna follow up in #557

fmvilas · 2021-05-14T12:13:33Z

Does this PR require a minor or a major semver version increment?

Don't worry about versioning in the PR. We handle the release process separately.

Should we stick with the JSON Merge Patch merge behavior?

I think we should. Inventing new algorithms will make it harder for tooling implementers. I agree that maybe we can reverse the order in which the merge is applied though. My two cents. Would love to know what others think.

github-actions · 2021-07-14T00:49:02Z

This pull request has been automatically marked as stale because it has not had recent activity 😴
It will be closed in 60 days if no further activity occurs. To unstale this pull request, add a comment with detailed explanation.
Thank you for your contributions ❤️

derberg · 2021-08-11T12:06:51Z

Proposal shared with the wider community:

LinkedIn
Twitter
Slack (need to join first- {invite link](https://www.asyncapi.com/slack-invite))

jessemenning · 2021-08-11T12:45:52Z

I am intrigued by the proposal, and if we move forward with a more reference based spec these rules will become increasingly important. I will admit I'm not smart enough to fully understand the implications though, would it be possible for @Fannon or others to present at a SIG, or another meeting?

Fannon · 2021-08-11T13:31:04Z

@jmenning-solace : Yes, this would benefit much from some explaining. I'd be happy to do that.
When is the next SIG meeting or a good occasion for it?

derberg · 2021-08-11T13:51:39Z

@Fannon next meeting is next week -> asyncapi/community#51

github-actions · 2021-12-17T01:00:13Z

This pull request has been automatically marked as stale because it has not had recent activity 😴

It will be closed in 120 days if no further activity occurs. To unstale this pull request, add a comment with detailed explanation.

There can be many reasons why some specific pull request has no activity. The most probable cause is lack of time, not lack of interest. AsyncAPI Initiative is a Linux Foundation project not owned by a single for-profit company. It is a community-driven initiative ruled under open governance model.

Let us figure out together how to push this pull request forward. Connect with us through one of many communication channels we established here.

Thank you for your patience ❤️

jonaslagoni · 2021-12-20T12:12:19Z

@Fannon you have listed a bunch of options and ways to solve this, however, as a reader, it is hard to keep track of all the pros and cons and understand the ripple effect (through tooling, etc).

I have a hard time figuring out what this PR is trying to fix, and I guess it comes down to which direction we should take it.

Since you have the knowledge, my suggestion would be that you pick the direction you would like to see AsyncAPI adopt, regardless of breaking change or not. See it as a perfect world, which option would then be best suited?

That way you can focus the PR on one suggestion that highlights the changes it requires, as well as letting others know why you chose it and why not one of the other options.

You can also do it in multiple stages, so you start by Do not change the current trait behavior and just describe it and it's limitations, and then work towards major version change where you suggest the new behavior you would like to see 🙂

I think this issue is definitely something that should be addressed, so thanks for taking the time!

Fannon · 2021-12-20T13:35:34Z

Hi @jonaslagoni,

yes, I see that this PR is hard to understand in its current state.
The problem is, we already discussed where we wanted to go and I wasn't sure if we already had agreement.
In this PR I described the compromise solution that I understood that AsyncAPI found best: Keep using the JSON merge path algorithm, change the order of merging and clearly define the previously undefined aspects.

Only in the code-example I have a second option (not using JSON merge patch, but an own definition of merge algorithm + implementatoin).

There is also one PR that only documents the status quo: #517

Best,
Simon

Fannon · 2022-01-12T07:25:07Z

So I've thought about this a bit more in another context.
My last state was that we didn't reach consensus and a decision on how to approach this.

In this PR I've tried to show a compromise solution, chaning the order of inheritance but keeping the JSON Merge Patch behavior. My personal opinion on this has changed even more in the direction that we should not use JSON Merge Patch for defining the merge behavior:

Like already stated, JSON merge patch is written and built for the purpose of patching (e.g. overwriting) the target objects. This has already led to some confusions
If we want to have more trait like or inheritance like behavior, we want to do basically the opposite. Of course, we can inverse the direction how the algorithm is applied, but this requires some inversions in your thinking :)
The JSON Merge patch has the feature of using a special null semantics, used for deleting properties in the target. If we keep this behavior and just apply it on the merged object instead of the target, we would do something that is not common in inheritance concepts. It even violates a well established principle, the Liskov Substitution Principle.

To sum it up: My proposal would be to define our own merge algorithm (It's not that much anyway) in written form and additionally as a reference code example, inkcl. unit tests with example data.
We still need to make up our mind how we interpret and explain the term trait. I understand that you can use traits to implement composition, but to me those are different things. Compositions are not using inheritance at all, they just add multiple aspects to one object and live side by side with each other. What we currently have with traits seems to be more like multi-parent inheritance / an object merge algorithm.

github-actions · 2022-05-13T00:20:28Z

This pull request has been automatically marked as stale because it has not had recent activity 😴

It will be closed in 120 days if no further activity occurs. To unstale this pull request, add a comment with detailed explanation.

There can be many reasons why some specific pull request has no activity. The most probable cause is lack of time, not lack of interest. AsyncAPI Initiative is a Linux Foundation project not owned by a single for-profit company. It is a community-driven initiative ruled under open governance model.

Let us figure out together how to push this pull request forward. Connect with us through one of many communication channels we established here.

Thank you for your patience ❤️

patrickp-at-work · 2022-07-14T10:21:36Z

Hello @fmvilas,
in #600, @clemensv had asked for a definition of traits. You had closed #600 as duplicate of this task which makes good sense to me. But I checked the PR and it doesn't really define traits or explain their use case(s). Can we still add that? As per my understanding, this issue has not changed the general idea of traits.

patrickp-at-work · 2022-07-14T13:32:14Z

I think we even already have a nice definition which was filed within #108:

Traits are pieces of information that will be merged into the referencing object.

We could extend this definition to:

Traits are pieces of information that will be merged into the referencing object. Using traits reduces the amount of repetitive code within an AsyncAPI document.

fmvilas · 2022-07-18T16:47:58Z

Yeah, as long as the PR is open we still have time 😄 In any case, I'm not the champion of this issue but @Fannon is.

patrickp-at-work · 2022-07-19T11:15:18Z

spec/asyncapi.md

@@ -139,6 +139,21 @@ This is applicable for `$ref` fields in the specification as follows from the [J

 By convention, the AsyncAPI Specification (A2S) file is named `asyncapi.json` or `asyncapi.yaml`.



The spec should define "traits" before explaining their behavior. My suggestion from the PR conversation:

### <a name="traits"></a>Traits Traits are pieces of information that will be merged into the referencing object. Using traits reduces the amount of repetitive code within an AsyncAPI document.

I've added this now to the PR just above the ### Trait Inheritance Section

@patrickp-at-work

as suggested by @patrickp-at-work

sonarcloud · 2022-08-10T04:40:18Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

Fannon · 2022-08-10T04:46:07Z

So how do we want to proceed with this?

Right now, this PR / issue is in a complicated state, because to me it is not really clear which direction we want to take and how this would be decided.

We've discussed a few options, here are the ones that I find most sensible / realistic.
Fore more detailed description, see the PR description:

Option C) Remove trait feature from AsyncAPI 3 as it introduces too much complexity
Option B) Keep trait feature, but change how it is applied and describe it
- Here we need to decide between the two B variants. My personal favorite would be B2, but this PR is following the B1 approach as this seemed to be the preference when we discussed it a while ago.

I can also imagine that with the restructuring of AsyncAPI 3, a few circumstances have changed and new insights have been gained. Maybe that also changes our perspective here ;)

github-actions · 2023-07-10T00:21:22Z

This pull request has been automatically marked as stale because it has not had recent activity 😴

It will be closed in 120 days if no further activity occurs. To unstale this pull request, add a comment with detailed explanation.

There can be many reasons why some specific pull request has no activity. The most probable cause is lack of time, not lack of interest. AsyncAPI Initiative is a Linux Foundation project not owned by a single for-profit company. It is a community-driven initiative ruled under open governance model.

Let us figure out together how to push this pull request forward. Connect with us through one of many communication channels we established here.

Thank you for your patience ❤️

smoya · 2023-08-11T14:34:24Z

Closing in favour of #907. Thanks @Fannon for the work you did here and that inspired the new traits merge mechanism 🤜✨🤛

Fannon and others added 2 commits April 29, 2021 15:48

Merge pull request #1 from asyncapi/master

9118ce9

update to origin master

Add section on trait inheritance and refer to it from trait objects

ef3975a

Fannon changed the title ~~fix: redefine the trait inheritance behavior~~ fix: redefine the trait inheritance behavior 💡 Apr 29, 2021

First draft how to formalize the trait inheritance rules

07ba172

Fannon changed the title ~~fix: redefine the trait inheritance behavior 💡~~ fix: redefine the trait inheritance behavior Apr 29, 2021

Fannon marked this pull request as draft April 29, 2021 16:38

Fannon mentioned this pull request Apr 29, 2021

fix: clarify that traits overwrite the objects they are applied to #517

Closed

Fannon added 2 commits April 30, 2021 08:23

Explicitly define behavior for undefined and null values

89322f8

Define what happens with undefined and null

f305df8

Fannon mentioned this pull request Apr 30, 2021

Application of message traits (intentionally) replacing existing attributes #505

Closed

Fannon force-pushed the trait-inheritance-proposal branch from 08d6644 to 1874ac7 Compare April 30, 2021 08:50

Updated description of trait inheritance merge behavior

06db043

Now it aims to be identical to JSON Merge patch

Fannon force-pushed the trait-inheritance-proposal branch from 1874ac7 to 06db043 Compare April 30, 2021 08:54

Fannon marked this pull request as ready for review April 30, 2021 09:25

Excplicitly state that must be dereferenced before merging

c747072

Added some further clarifications to avoid target object naming confusion

fmvilas reviewed May 14, 2021

View reviewed changes

fmvilas added the 💡 Proposal (RFC 1) RFC Stage 1 (See CONTRIBUTING.md) label May 14, 2021

github-actions bot added the stale label Jul 14, 2021

magicmatatjahu removed the stale label Jul 14, 2021

This was referenced Aug 12, 2021

AsyncAPI open meeting, Tuesday August 17 2021 4PM UTC asyncapi/community#51

Closed

Explain "traits" and their use-cases #600

Closed

github-actions bot added the stale label Dec 17, 2021

jonaslagoni mentioned this pull request Dec 17, 2021

Clearly define how traits should be applied asyncapi/parser-api#36

Closed

github-actions bot removed the stale label Dec 21, 2021

jonaslagoni mentioned this pull request Jan 18, 2022

Spec 3.0 Meeting, 16 UTC Wednesday January 19 2022 asyncapi/community#235

Closed

github-actions bot added the stale label May 13, 2022

fmvilas removed the stale label May 17, 2022

Fannon changed the base branch from master to next-major-spec June 21, 2022 04:35

Fannon requested review from derberg, dalelane and asyncapi-bot-eve as code owners June 21, 2022 04:35

patrickp-at-work reviewed Jul 19, 2022

View reviewed changes

Added trait explanation

6f57e01

as suggested by @patrickp-at-work

fmvilas force-pushed the next-major-spec branch from 8805c88 to 624ad52 Compare September 22, 2022 13:38

magicmatatjahu mentioned this pull request Feb 16, 2023

feat: new traits merge mechanism #907

Merged

github-actions bot added the stale label Jul 10, 2023

smoya closed this Aug 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: redefine the trait inheritance behavior #532

fix: redefine the trait inheritance behavior #532

Fannon commented Apr 29, 2021 •

edited

Loading

sonarcloud bot commented May 1, 2021

fmvilas May 14, 2021

Fannon May 14, 2021

Fannon May 14, 2021

jonaslagoni Dec 17, 2021 •

edited

Loading

jonaslagoni Dec 17, 2021

Fannon Dec 17, 2021

jonaslagoni Dec 17, 2021

fmvilas commented May 14, 2021

github-actions bot commented Jul 14, 2021

derberg commented Aug 11, 2021

jessemenning commented Aug 11, 2021

Fannon commented Aug 11, 2021

derberg commented Aug 11, 2021

github-actions bot commented Dec 17, 2021

jonaslagoni commented Dec 20, 2021 •

edited

Loading

Fannon commented Dec 20, 2021

Fannon commented Jan 12, 2022

github-actions bot commented May 13, 2022

patrickp-at-work commented Jul 14, 2022

patrickp-at-work commented Jul 14, 2022

fmvilas commented Jul 18, 2022

patrickp-at-work Jul 19, 2022

Fannon Aug 10, 2022

sonarcloud bot commented Aug 10, 2022

Fannon commented Aug 10, 2022

github-actions bot commented Jul 10, 2023

smoya commented Aug 11, 2023 •

edited

Loading


		When traits ([Operation Trait Object](#operationTraitObject) and [Message Trait Object](#messageTraitObject)) are defined, they MUST be merged into the target object according to the following rules:

		* Traits are merged with a recursive merge algorithm, as defined in the [JSON Merge Patch](https://tools.ietf.org/html/rfc7386) RFC. To summarize the main merge rules as a merge from source object into a target object:

		@@ -139,6 +139,21 @@ This is applicable for `$ref` fields in the specification as follows from the [J

		By convention, the AsyncAPI Specification (A2S) file is named `asyncapi.json` or `asyncapi.yaml`.

fix: redefine the trait inheritance behavior #532

fix: redefine the trait inheritance behavior #532

Conversation

Fannon commented Apr 29, 2021 • edited Loading

title: Redefine and explicitly formalize the trait inheritance behavior

Related Material / Inspiration

Open Questions / Problems

sonarcloud bot commented May 1, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonaslagoni Dec 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fmvilas commented May 14, 2021

github-actions bot commented Jul 14, 2021

derberg commented Aug 11, 2021

jessemenning commented Aug 11, 2021

Fannon commented Aug 11, 2021

derberg commented Aug 11, 2021

github-actions bot commented Dec 17, 2021

jonaslagoni commented Dec 20, 2021 • edited Loading

Fannon commented Dec 20, 2021

Fannon commented Jan 12, 2022

github-actions bot commented May 13, 2022

patrickp-at-work commented Jul 14, 2022

patrickp-at-work commented Jul 14, 2022

fmvilas commented Jul 18, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Aug 10, 2022

Fannon commented Aug 10, 2022

github-actions bot commented Jul 10, 2023

smoya commented Aug 11, 2023 • edited Loading

Fannon commented Apr 29, 2021 •

edited

Loading

jonaslagoni Dec 17, 2021 •

edited

Loading

jonaslagoni commented Dec 20, 2021 •

edited

Loading

smoya commented Aug 11, 2023 •

edited

Loading